Added defaultValue to ManualDecorators. #258
Conversation
tests/linkcommand.js
Outdated
| @@ -324,25 +332,33 @@ describe( 'LinkCommand', () => { | |||
|
|
|||
| expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url</$text>]bar' ); | |||
| } ); | |||
|
|
|||
| it( 'should add additional attributes to link if those are falsy but decorator\'s defaultValue is set to true', () => { | |||
There was a problem hiding this comment.
Why storing the false value in the model? Also, how do you actually remove the value from the model?
There was a problem hiding this comment.
As I see this – the only thing that needs to change when defalutValue is true is the state of this button in the UI when you create a new link. Since it'll be on at that point, the command will be executed with isSth:true and then our job's done. The rest will work the way it works so far. Do I miss something?
There was a problem hiding this comment.
As discussed on a call:
LinkFormViewis binding itsisOnproperty tovalueof manual decorator,valueof a manual decorator is updated every timeLinkCommandisrefreshed,- every time
valueis extracted from the model's attribute, - we need to store
falsein the attribute to indicate that user already decided whether the manual decorator is supposed to be applied or not
tests/linkcommand.js
Outdated
| @@ -324,25 +332,33 @@ describe( 'LinkCommand', () => { | |||
|
|
|||
| expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url</$text>]bar' ); | |||
| } ); | |||
|
|
|||
| it( 'should add additional attributes to link if those are falsy but decorator\'s defaultValue is set to true', () => { | |||
There was a problem hiding this comment.
Why storing the false value in the model? Also, how do you actually remove the value from the model?
There was a problem hiding this comment.
I checked that I correctly read the code – it indeed puts the false value in the model.
|
Based on manual tests all work fine. |
|
After looking at this PR again, I still have doubts regarding keeping the false value in the model. Isn't it equal in meaning to the lack of the attribute? The problem is that a decorator which default value is false has two modes in the model:
The decorator which default value is true has 3 modes:
Unfortunately, from the model perspective this is such a big inconsistency, that I'd work on resolving it. For instance, a link might've been inserted by a tool which knows nothing about decorators. In this case, it will insert a link with just To sum up – there should always be only 2 modes. In case of decorators which default value is set to true:
I know that how the UI is implemented makes this tricky, but there's definitely some option there. |
src/linkediting.js
Outdated
| } | ||
|
|
||
| // We must check if all decorator values are set on view element. | ||
| const decoratorValue = Object.entries( decorator.attributes ).every( ( [ key, value ] ) => { |
There was a problem hiding this comment.
I wanted to write that this comparison is not precise enough because you can have two decorators using the class attribute but I can see that this is not supported already. It's a pity because the engine treats each class separately (styles and classes are special types of attributes) and this could work. In fact, getData() works, but setData() breaks.
There was a problem hiding this comment.
I reported a ticket for that in ckeditor/ckeditor5#6571.
src/linkediting.js
Outdated
| return; | ||
| } | ||
|
|
||
| // We must check if all decorator values are set on view element. |
There was a problem hiding this comment.
| // We must check if all decorator values are set on view element. | |
| // We must check if all decorator values are set on view element. | |
| // The comparison is naive (doesn't handle granular classes or styles) | |
| // but link decorators don't support them anyway (#6571). |
Suggested merge commit message (convention)
Feature: Added defaultValue to ManualDecorators. Closes ckeditor/ckeditor5#6031.
Additional information